Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(web): install button and confirmation dialog fixes #1717

Merged
merged 5 commits into from
Nov 4, 2024

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Oct 30, 2024

Problems

  1. The "Install" button is shown during the installation progress 😥

    Thanks @imobachgs!

  2. The install confirmation dialog could be accepted by mistake because of 'Continue' button receiving the focus by default

    https://trello.com/c/8ZWINXjs (internal/private link)

Solutions

  1. Do not show the "Install" button when it does not make sense
  2. Set 'Cancel' button as focused by default via autofocus attribute (a.k.a. autoFocus prop)

Testing

  • New behavior covered by unit tests.

Notes

Constants for router paths were moved to a single routes/paths file in order to avoid circular dependencies that made test suite fail after fe18300

Since it does not make sense to render the InstallButton when the
installation is in progress or finished. The same apply for the login
screen, product selection, and product progress.
Changing the default focus in the confirmation dialog from 'Continue' to
'Cancel' to avoid the user triggering the installation because
accidentally sending the {enter} keystroke.
@dgdavid dgdavid changed the title Fix install button fix(web): install button and confirmation dialog fixes Oct 30, 2024
@@ -48,7 +63,7 @@ according to the provided installation settings.",
{/* TRANSLATORS: button label */}
{_("Continue")}
</Popup.Confirm>
<Popup.Cancel onClick={onClose}>
<Popup.Cancel autoFocus onClick={onClose}>
Copy link
Contributor Author

@dgdavid dgdavid Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: I'm aware of https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/autofocus#accessibility_concerns but I have slightly tested it with Orca screen reader and it seems to work as expected. No tools for recording it at this moment, sorry.

In any case, let's think on this a as hotfix for preventing users starting the installation by accident and improve it later when re-working the confirmation dialog (https://trello.com/c/SKhWWsAk - private link)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, let's think on this a as hotfix for preventing users starting the installation by accident and improve it later when re-working the confirmation dialog (https://trello.com/c/SKhWWsAk - private link)

Or evaluate if it has more advantages to relay in the Modal#elementToFocus prop, which was added more than a year ago but I haven't been aware until today while checking the recently released PatternFly v6.

But please let's postpone such an evaluation to the moment we migrate to v6 and apply the decided approach to all dialogs needing to force the focus.

@dgdavid dgdavid merged commit 7ab5690 into master Nov 4, 2024
6 checks passed
@dgdavid dgdavid deleted the fix-install-button branch November 4, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants